Skip to content

Conversation

reem
Copy link
Contributor

@reem reem commented Sep 8, 2014

Introduces two Phantom Types, Fresh and Streaming, which indicate the status
of a Response.

Response::start translates an Response into a Response by
writing the StatusCode and Headers.

Response allows modification of Headers and StatusCode, but does
not allow writing to the body. Response has the opposite privileges.

Disadvantages:

Uses a Phantom Type, which some may find confusing.

Alternatives:

Continue to track this dynamically and provide publicly accessible methods for
conditionally updating Headers and StatusCode and disallow direct access or
mutation.

Keep doing what we do now, but expose headers_written.

Split Response into two structs and create a Response trait. This introduces
code duplication and new ways to interact with Headers and Status.

Fixes #6

@reem reem changed the title Statically track the status of a Response by splitting UnwrittenResponse Statically track the status of a Response by splitting Response Sep 8, 2014
@reem reem force-pushed the static-response-states branch from aa3f57d to 1aab0fd Compare September 8, 2014 22:20
@reem reem changed the title Statically track the status of a Response by splitting Response Statically track the status of a Response by using a Phantom Type Sep 8, 2014
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left over *Response from a previous attempt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Will rebase.

@seanmonstar
Copy link
Member

I really quite like this idea. I want the compiler to catch when you try to set a header after writing. Dynamic is slower, and has errors show up in actual programs instead.

If I merge this and #17, then Response's will be generic over 2 things, right? Does this feel painful as a framework dev?

reem added 2 commits September 8, 2014 16:52
Introduces two Phantom Types, Fresh and Streaming, which indicate the status
of a Response.

Response::start translates an Response<Fresh> into a
Response<Streaming> by writing the StatusCode and Headers.

Response<Fresh> allows modification of Headers and StatusCode, but does
not allow writing to the body. Response<Streaming> has the opposite privileges.
@reem reem force-pushed the static-response-states branch from 1aab0fd to 13bb07e Compare September 8, 2014 23:52
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never been a fan of assignment via derefing a pointer... What if it were fn set_status(StatusCode)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I think people in Rust are used to assign-via-deref because this is a relatively common pattern, but I'm not opposed to set_status in principle. I wonder if there is a style guide convention for this case.

@reem
Copy link
Contributor Author

reem commented Sep 8, 2014

We should investigate if making #17 use dynamic dispatch creates a material difference in performance, if so, then yes, that sounds a tiny bit annoying. However, I think it's useful enough to justify it.

Most frameworks will probably not expose Request and Response directly, so I don't think it's too big of an issue. It's mostly useful for testing so one can mock Incoming, though it would probably be easier to just make http requests with a good client.

seanmonstar added a commit that referenced this pull request Sep 9, 2014
Statically track the status of a Response by using a Phantom Type
@seanmonstar seanmonstar merged commit e32845c into hyperium:master Sep 9, 2014
@reem reem deleted the static-response-states branch September 9, 2014 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refine strategy surrounding writing headers to a response
2 participants